-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make api_modify to ignore builtin items #130
make api_modify to ignore builtin items #130
Conversation
Signed-off-by: Tomas Herfert <herfik>
Codecov Report
@@ Coverage Diff @@
## main #130 +/- ##
==========================================
- Coverage 87.56% 87.44% -0.13%
==========================================
Files 28 28
Lines 3499 3505 +6
Branches 619 622 +3
==========================================
+ Hits 3064 3065 +1
- Misses 308 310 +2
- Partials 127 130 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
If it affects any path supported yet, this would be a breaking change and we cannot merge it. For If any existing path supports it for |
I'll I'll go through all the supported paths and let you know.
Makes sense.
In this case, we could maybe implement this later - perhaps in a new major version? would breaking change be allowed in such case? The reason why I'm thinking about it is that the builtin entries are hardcoded in ROS - those are not configurable by the user. Unlike the "default" entries which are defined by default and cannot be deleted, but the user can modify them. So defining a parametr to skip the builtin entries would really only cover the case when |
Thanks a lot!
While a new major version allows breaking changes, doing that without a sufficiently long deprecation period is something we really try to avoid in the Ansible world. So we should add the parameter with the current default, wait a few releases, deprecate the default, and then we can switch over to the other default in the next major release (giving users some time where they can optionally state their intention, then some time where they see a deprecation warning when they don't explicitly set the value, while they can also specify that option when some colleagues are using the same playbooks with certain older versions of this collection, since the option was already there for some time).
I fully understand (and I wish I would have added that code right from the beginning) :) |
Looks good. None of the supported paths (of the released version) have any builtins. See below:
it gave me the same result on both ROS 6 and 7:
You can try yourself. Some of the paths are failing, but that's because those don't exist either on ROS6 or ROS7. |
Great news! :) |
Signed-off-by: Tomas Herfert <herfik>
|
Signed-off-by: Tomas Herfert <herfik>
Co-authored-by: Felix Fontein <[email protected]>
@therfert thanks for yet another contribution :) |
SUMMARY
api_modify to ignore built-in items
ISSUE TYPE
COMPONENT NAME
api_modify
ADDITIONAL INFORMATION
this allows to use
handle_absent_entries: remove
with paths wherebuiltin
entries exist - e.g.interface list
without the need of explicitly defining those entries in thedata
module parametr as a workaround.This might be potentially breaking change (for those who used the workaround above) as far as any of the supported paths in the released version contains
builtin
entries.I don't know if it is the case or not. (the
interface list
was added recently and not released yet). I could potentially loop through all the supported paths to see if there are any builtin entries to confirm this if needed.If this is going to be approved, let me know if you would like to implement the same in the api_info module (if so - do you want to make it configurable via separate parameter or just cover it by the
indlude_dynamic
parameter?) .